fix: don't break on --substitute values which have commas #580
Conversation
The following: stacker build --substitute a=b,c used to work, until commit 3baba64: fix(ci): convert ci is failing due to perms (project-stacker#439) This squashed PR included 'chore(go.mod): update cli to github.com/urfave/cli/v2'. Switching from urfave/cli/v1 to v2 changes string slice flag behavior to automatically split on commas. Luckily there is an app.DisableSliceFlagSeparator flag we can set to tell it not to do that. Set that flag. And add a test for this. Signed-off-by: Serge Hallyn <shallyn@cisco.com>
hallyn
force-pushed
the
2024-01-05/fix-comma-subst
branch
from
4af746a to
d9cb119
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #580 +/- ##
==========================================
+ Coverage 57.10% 57.12% +0.01%
==========================================
Files 64 64
Lines 7519 7521 +2
==========================================
+ Hits 4294 4296 +2
Misses 2483 2483
Partials 742 742 ☔ View full report in Codecov by Sentry. |
@rchincha is this a regression in this test? (i don't see how it could be, but...) Or has it been seen elsewhere? |
| run: | | ||
| touch /foo | ||
| EOF | ||
| stacker build --substitute "a=b,c" |
There was a problem hiding this comment.
How is this substitution used though in the stacker.yaml?
There was a problem hiding this comment.
Do you mean in the testcase? It's not. That's not necessary to trigger the regression failure-to-build in main branch.
There was a problem hiding this comment.
Hm. When I run this test locally (sudo bats test/basic.bats), i get zero failures.
There was a problem hiding this comment.
In the run: section, maybe echo "${{a}}"?
There was a problem hiding this comment.
No, I was wrong - I can reproduce this. No idea why.
There was a problem hiding this comment.
The CI failure is due to the gzip issue. It is a concern, but unrelated to this PR.
There was a problem hiding this comment.
Actually it seems to just be a flaky test? :
ubuntu@stacker:~/stacker$ sudo -E bats test/gzip.bats
✓ import various sizes
1 test, 0 failures
The following:
stacker build --substitute a=b,c
used to work, until commit 3baba64:
fix(ci): convert ci is failing due to perms (#439)
This squashed PR included 'chore(go.mod): update cli to github.com/urfave/cli/v2'. Switching from urfave/cli/v1 to v2 changes string slice flag behavior to automatically split on commas.
Luckily there is an app.DisableSliceFlagSeparator flag we can set to tell it not to do that. Set that flag. And add a test for this.
What type of PR is this?
Which issue does this PR fix:
What does this PR do / Why do we need it:
If an issue # is not available please add repro steps and logs showing the issue:
Testing done on this change:
Automation added to e2e:
Will this break upgrades or downgrades?
Does this PR introduce any user-facing change?:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.